Skip to content

fix(review-filter): apply language for review filter stage#297

Closed
ScarletCarpet wants to merge 1 commit into
alibaba:mainfrom
ScarletCarpet:fix/review-filter-language
Closed

fix(review-filter): apply language for review filter stage#297
ScarletCarpet wants to merge 1 commit into
alibaba:mainfrom
ScarletCarpet:fix/review-filter-language

Conversation

@ScarletCarpet

Copy link
Copy Markdown
Contributor

Description

the review filter stage was not respond in language for some model who deny the rule of without any explanation

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: No comments generated. Looks good to me.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Thanks a lot for digging into the review-filter behavior, @ScarletCarpet — it's a genuinely subtle area and the change itself is clean (nice nil-guard and test coverage). 🙏

After looking closely, though, I don't think this is the right layer for the fix, so I'm going to close it. Here's the reasoning:

The review-filter output is a machine-parsed control signal, not user-facing text. The prompt asks the model to return a bare JSON array of comment IDs, and the consumer (executeReviewFilterparseFilterResponse) does StripMarkdownFences + json.Unmarshal and then just calls RemoveByPathAndIndices. Nothing this task emits is ever shown to the user — the comments that survive filtering were already produced by MAIN_TASK, which already carries the language directive.

Because of that, injecting Always respond in <language>. here has no user-visible field to act on, and it can actually work against us:

  • It nudges the model to emit prose/explanation, which directly contradicts the prompt's own "without any explanation" rule.
  • Any surrounding prose makes json.Unmarshal fail, so parseFilterResponse returns nil and the filter silently becomes a no-op.

Contrast with DedupTask: it may look like precedent since ScanTemplate.ApplyLanguage applies the directive to dedup too — but dedup emits merged_content, a genuinely user-facing field, so localizing it there is correct. The review filter has no such field, which is exactly why it's different.

Overlap with #295: that PR moves the filter to function calling specifically to stop models from emitting natural language. This change pushes in the opposite direction, so if #295 lands the language directive becomes moot anyway.

If the real symptom is chatty models breaking the parse, the more robust fixes are the function-calling approach in #295 or more lenient JSON extraction inside parseFilterResponse. If you've hit a concrete case where filter output actually reaches the user, please do reopen and point me at that path — I couldn't find one, and it would change the assessment.

Thanks again for the contribution and for the careful tests — really appreciate the effort here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants